Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[kjobctl] Move common SLURM_envs to container env vars. #3285

Closed
wants to merge 1 commit into from
Closed

[kjobctl] Move common SLURM_envs to container env vars. #3285

wants to merge 1 commit into from

Conversation

mbobrovskyi
Copy link
Contributor

@mbobrovskyi mbobrovskyi commented Oct 22, 2024 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Move common SLURM_ envs to container env vars on kjobctl.

Which issue(s) this PR fixes:

Prepare #3269

Special notes for your reviewer:

We have many variables that are the same in each container, so there's no need to generate them in the init container. Instead of that we can set it as a container env vars on builder.

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 22, 2024
@mbobrovskyi mbobrovskyi marked this pull request as draft October 22, 2024 15:11
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 22, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mbobrovskyi
Once this PR has been reviewed and has the lgtm label, please assign trasc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024
@k8s-ci-robot k8s-ci-robot requested review from mimowo and trasc October 22, 2024 15:11
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2024
@netlify Netlify
Copy link

netlify bot commented Oct 22, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit f1b4e3c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/671876ba1623560008b06e04

@mwielgus
Copy link
Contributor

I'm afraid this will make the already big slurm job/pod definition significantly bigger. Is why is it needed?

@mbobrovskyi
Copy link
Contributor Author

mbobrovskyi commented Oct 22, 2024 •
edited
Loading

Is why is it needed?

We discussed on the previous weekly meeting to create init container on go. Before that trying to optimize init script.

@mbobrovskyi
Copy link
Contributor Author

In another way we need to create a lot of flags for go slurm-init to send all of this values.

@mimowo @trasc WDYT?

@mbobrovskyi
Copy link
Contributor Author

Another approach I also can see to get it from configmap.

@mwielgus
Copy link
Contributor

Configmap sounds ok.

@mbobrovskyi mbobrovskyi force-pushed the cleanup/move-slurm-envs-to-container-env-vars branch 3 times, most recently from 9311909 to e063352 Compare October 22, 2024 18:56
@mbobrovskyi mbobrovskyi changed the title Move common SLURM_envs to container env vars. [kjobctl] Move common SLURM_envs to container env vars. Oct 22, 2024
@mbobrovskyi mbobrovskyi force-pushed the cleanup/move-slurm-envs-to-container-env-vars branch 2 times, most recently from 4a95ab0 to 8e6cd4b Compare October 23, 2024 03:39
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2024
@mbobrovskyi mbobrovskyi force-pushed the cleanup/move-slurm-envs-to-container-env-vars branch from 8e6cd4b to f1b4e3c Compare October 23, 2024 04:08
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 23, 2024
@trasc
Copy link
Contributor

trasc commented Oct 23, 2024

In another way we need to create a lot of flags for go slurm-init to send all of this values.

@mimowo @trasc WDYT?

I don't think that loading the env vars from a = separated file is such a big challenge in a go app (split by \n then by =). So, at least for now, we should not add an extra config map just for this.

@mbobrovskyi
Copy link
Contributor Author

/close

In the case where we are using the current ConfigMap, it will be addressed in #3293.

@k8s-ci-robot
Copy link
Contributor

@mbobrovskyi: Closed this PR.

In response to this:

/close

In the case where we are using the current ConfigMap, it will be addressed in #3293.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimowo mimowo Awaiting requested review from mimowo

@trasc trasc Awaiting requested review from trasc

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mbobrovskyi @k8s-ci-robot @mwielgus @trasc